Remove setRotationCenter API #581
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adroitwhiz
force-pushed
the
remove-set-rotation-center
branch
from
ad2af11 to
dd19d0e
Compare
fsih
reviewed
Jun 22, 2020
| const changed = ( | ||
| toFloat32(x) !== this._rotationCenter[0] || | ||
| toFloat32(y) !== this._rotationCenter[1]); | ||
| if (!emptySkin && changed) { |
There was a problem hiding this comment.
Note: this is replaced by https://github.com/LLK/scratch-render/pull/561/files
fsih
reviewed
Jun 22, 2020
| @@ -1444,9 +1418,6 @@ class RenderWebGL extends EventEmitter { | |||
| if ('skinId' in properties) { | |||
| this.updateDrawableSkinId(drawableID, properties.skinId); | |||
| } | |||
| if ('rotationCenter' in properties) { | |||
There was a problem hiding this comment.
We might want to document the properties, but not necessarily in this PR
There was a problem hiding this comment.
I guess if this function is going away it doesn't matter
|
FYI test is falling because playwright only supports node 10+ but it's still node 8 |
adroitwhiz
force-pushed
the
remove-set-rotation-center
branch
from
dd19d0e to
d9c854f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves
A step on the long-ish path towards fixing #550
Also a step towards fixing #570
Proposed Changes
This PR removes the API for setting a drawable's rotation center and the
Skin.setRotationCenterfunction.It does not remove the API for setting a skin's rotation center when setting its contents.
To fix unit tests which previously called
Skin.setRotationCenter, it also adds arotationCentersetter (plus corresponding getter) toMockSkin, which the unit tests now use.I'm not expecting this to be merged right away--in case scratchfoundation/scratch-vm#2314 causes any regressions in the near future, it'll probably be beneficial to be able to revert that without having to re-add the rotation center API here as well.
Reason for Changes
In order to fix (no, GitHub, it doesn't close) #550, vector costumes' SVG viewboxes must depend on their rotation centers. Every time the viewbox of an SVG is set, its
<img>must be updated as well, which is a fundamentally asynchronous operation.As such, a synchronous API for setting a skin's rotation center after the skin is loaded is incompatible with the changes necessary for #550.
With scratchfoundation/scratch-vm#2314 merged, that synchronous API is no longer used and can be removed.